Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dockerfile: use bats-core #2307

Closed
wants to merge 1 commit into from

Conversation

kolyshkin
Copy link
Contributor

The bats testing framework we use for integration test is not maintained
since 2015 and was superceded by bats-core [1]. Also, we are using an
unreleased version (and as far as I remember depend on its functionality,
although I don't remember what is it exactly).

Unfortunately, Debian still packages very old version of bats from the old repo.

Use recent bats-core as installed by npm, as recommended in [3].

[1] sstephenson/bats#269
[3] https://github.com/bats-core/bats-core/wiki/Install-Bats-Using-a-Package

Signed-off-by: Kir Kolyshkin [email protected]

The bats testing framework we use for integration test is not maintained
since 2015 and was superceded by bats-core [1]. Unfortunately, Debian
still packages very old version of bats from the old repo.

Use recent bats-core as installed by npm, as recommended in [3].

[1] sstephenson/bats#269
[3] https://github.com/bats-core/bats-core/wiki/Install-Bats-Using-a-Package

Signed-off-by: Kir Kolyshkin <[email protected]>
Dockerfile Outdated
@@ -33,6 +32,7 @@ RUN dpkg --add-architecture armel \
libseccomp-dev:armhf \
libseccomp-dev:ppc64el \
libseccomp2 \
npm \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this doesn't use the same indentation (tabs vs. spaces) as the surrounding lines.

&& git reset --hard "${BATS_VERSION}" \
&& ./install.sh /usr/local \
&& rm -rf /tmp/bats
RUN npm install -g bats
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason for using npm for installing, and not from GitHub? the old method (with updated repo) should still work; https://github.com/bats-core/bats-core#installing-bats-from-source

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because it's simpler and someone (hopefully) maintains the package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan; not only would we then depend on the npm package maintainer to maintain the package (in addition to the new bats-core org), it also adds a whole bunch of new dependencies just to install bats;

docker build -t foo -<<EOF
FROM golang:1.13.10-buster
RUN apt-get update && apt-get install -y --no-install-recommends npm && apt-get clean && rm -rf /var/cache/apt /var/lib/apt/lists/*
EOF

docker history foo
IMAGE               CREATED             CREATED BY                                      SIZE                COMMENT
947f3c9961e5        5 seconds ago       RUN /bin/sh -c apt-get update && apt-get ins…   82.6MB              buildkit.dockerfile.v0
<missing>           5 days ago          /bin/sh -c #(nop) WORKDIR /go                   0B
...

That's 83MB to install npm, which we only need to install bats

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently setup bats for a project, installing from the github repo was very simple. I would rather us do that instead of using npm

@kolyshkin
Copy link
Contributor Author

Will install from git

@kolyshkin kolyshkin closed this Apr 21, 2020
@kolyshkin kolyshkin deleted the bats-core branch April 21, 2020 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants